ref(grouping): Simplify handling of variant name and exception data #97458
+31
−19
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
^ Conflicts: ^ src/sentry/grouping/strategies/message.py
github-actions
bot
added
the
Scope: Backend
yuvmen
approved these changes
Aug 8, 2025
| @@ -27,9 +27,6 @@ | |||
| ], | |||
| delegates=["frame:v1", "stacktrace:v1", "single-exception:v1"], | |||
| initial_context={ | |||
| # This key in the context tells the system which variant should | |||
| # be produced. TODO: phase this out. | |||
There was a problem hiding this comment.
ah the joy of removing a 4 year old TODO in code :)
| @@ -72,15 +72,17 @@ def normalize_message_for_grouping(message: str, event: Event) -> str: | |||
| def message_v1( | |||
| interface: Message, event: Event, context: GroupingContext, **kwargs: Any | |||
| ) -> ReturnedVariants: | |||
| variant_name = context["variant_name"] | |||
There was a problem hiding this comment.
I know the code previously didnt, but should we not asserting for existence here as well?
There was a problem hiding this comment.
The produces_variants decorator (indirectly) sets variant_name in the context, so we know it will be there.
(To be more precise, the decorator calls call_with_variants, and call_with_variants sets variant_name immediately before calling strategy_func, which in this case is the function message_v1 that you're looking at.)
andrewshie-sentry
pushed a commit
that referenced
this pull request
Aug 12, 2025
…97458) During grouping, we pass data from one component strategy to another by storing it in the grouping context. In that context, two values - the variant name and a blob of exception data - are only sometimes defined. Until recently, the `GroupingContext` class supported key-lookup using subscript notation (`context[some_key]`) the same way a dictionary would, but lacked the safer `.get()` method. We therefore currently initialize both values to `None`, to prevent raising a `KeyError` if the values haven't been set. Now that `.get()` has been implemented, though, we don't need to do that. This PR therefore switches the way we access them to use `.get()`, and removes the default initialization. In order to enforce when `variant_name` is or isn't expected to be defined, it also extends the practice already followed in some strategies of asserting on its existence (or lack thereof). This ensures that if we ever make changes to the grouping code, we'll know right away if our assumptions have been violated.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
During grouping, we pass data from one component strategy to another by storing it in the grouping context. In that context, two values - the variant name and a blob of exception data - are only sometimes defined. Until recently, the
GroupingContextclass supported key-lookup using subscript notation (context[some_key]) the same way a dictionary would, but lacked the safer.get()method. We therefore currently initialize both values toNone, to prevent raising aKeyErrorif the values haven't been set.Now that
.get()has been implemented, though, we don't need to do that. This PR therefore switches the way we access them to use.get(), and removes the default initialization. In order to enforce whenvariant_nameis or isn't expected to be defined, it also extends the practice already followed in some strategies of asserting on its existence (or lack thereof). This ensures that if we ever make changes to the grouping code, we'll know right away if our assumptions have been violated.